Skip to content

Use IdentityHashMap for consistent JIT optimization in AbstractCompositeMeter #6846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use IdentityHashMap for consistent JIT optimization in AbstractCompositeMeter #6846

wants to merge 2 commits into from
+132 −5

Conversation

@belugabehr
Copy link

@belugabehr belugabehr commented Nov 1, 2025

Replace Collections.emptyMap() with new IdentityHashMap to maintain consistent map implementation type throughout the object's lifecycle, enabling JVM scalar replacement optimization and reducing iterator allocation overhead.

Fixes gh-6811

@belugabehr belugabehr force-pushed the belugabehr/composite-id-map branch 2 times, most recently from 7ea05ba to 3edc0a9 Compare November 1, 2025 02:56
@belugabehr belugabehr force-pushed the belugabehr/composite-id-map branch 2 times, most recently from a55c2da to 1e39d3f Compare November 18, 2025 03:34
@belugabehr
Copy link
Author

belugabehr commented Nov 18, 2025 •
edited
Loading

@jonatan-ivanov - Made suggested change. Confirmed the allocations have stopped: #6811 (comment)

@shakuzen
Copy link
Member

shakuzen commented Nov 18, 2025

@belugabehr could you share the code for the benchmark you're using?

@belugabehr
Copy link
Author

belugabehr commented Nov 18, 2025

@shakuzen Details are posted in: gh-6811

Thanks!

shakuzen
shakuzen reviewed Nov 19, 2025
View reviewed changes
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is fine as is, but it's also brittle. I wonder if we should try adding a test that asserts there is no allocation after C2 compilation. The problem is that we only have heuristics to guess when C2 compilation is done - by default in most modern JVMs it starts after 10,000 invocations of a method.

It may be helpful to commit the benchmarks along with this, but benchmarks don't get run with the build so they are not a guarantee this won't regress with some other change.

I think there is larger refactoring to consider around this, but I think it's okay to consider that after merging this.

I also think we should pursue adding a warning log so that users who can do all setup before registering metrics can avoid surprises like this and others (such as the values being recorded before a non-composite registry is added being dropped).

@belugabehr
Copy link
Author

belugabehr commented Nov 19, 2025

I also think we should pursue adding a warning log so that users who can do all setup before registering metrics can avoid surprises like this and others (such as the values being recorded before a non-composite registry is added being dropped).

Agreed!

@shakuzen shakuzen mentioned this pull request Nov 20, 2025
Open
@shakuzen
Copy link
Member

shakuzen commented Nov 20, 2025

I also think we should pursue adding a warning log so that users who can do all setup before registering metrics can avoid surprises like this and others (such as the values being recorded before a non-composite registry is added being dropped).

I've opened #6908 for this.

@belugabehr belugabehr force-pushed the belugabehr/composite-id-map branch from 1e39d3f to 23bd25c Compare November 20, 2025 16:40
@belugabehr
Copy link
Author

belugabehr commented Nov 20, 2025

@shakuzen - PR updated with your suggestions. Thanks for all the help! Excited to get this over the line.

@belugabehr belugabehr force-pushed the belugabehr/composite-id-map branch from 23bd25c to a0f5c74 Compare November 21, 2025 14:53
@belugabehr @jonatan-ivanov
Use IdentityHashMap for consistent JIT optimization in AbstractCompos…
eb4257a 
…iteMeter

Replace Collections.emptyMap() with new IdentityHashMap<>() to maintain
consistent map implementation type throughout the object's lifecycle,
enabling JVM scalar replacement optimization and reducing iterator
allocation overhead.

Fixes micrometer-metricsgh-6811

Signed-off-by: David Mollitor <[email protected]>
@jonatan-ivanov jonatan-ivanov force-pushed the belugabehr/composite-id-map branch from a0f5c74 to bf27aaf Compare November 22, 2025 00:02
@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Nov 22, 2025 •
edited
Loading

I added CompositeScalarReplacementBenchmark and rebased the PR on latest main.
For the record, here are sample executions before and after this fix:

Benchmark                                      Mode  Cnt     Score   Error   Units

#before
compositeAndEmptyComposite                     avgt          6.797           ns/op
compositeAndEmptyComposite:gc.alloc.rate       avgt       5612.229          MB/sec
compositeAndEmptyComposite:gc.alloc.rate.norm  avgt         40.000            B/op
compositeAndEmptyComposite:gc.count            avgt        101.000          counts
compositeAndEmptyComposite:gc.time             avgt         39.000              ms

#after
compositeAndEmptyComposite                     avgt          4.722           ns/op
compositeAndEmptyComposite:gc.alloc.rate       avgt          0.002          MB/sec
compositeAndEmptyComposite:gc.alloc.rate.norm  avgt         ≈ 10⁻⁵            B/op
compositeAndEmptyComposite:gc.count            avgt            ≈ 0          counts
(gc.time is missing)

Please notice gc.alloc.rate.

@jonatan-ivanov jonatan-ivanov force-pushed the belugabehr/composite-id-map branch from bf27aaf to 5ea598a Compare November 22, 2025 00:12
@jonatan-ivanov jonatan-ivanov mentioned this pull request Nov 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shakuzen shakuzen shakuzen left review comments

@jonatan-ivanov jonatan-ivanov jonatan-ivanov approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Collections Empty Map Defeats JIT Scalar Replacement for AbstractCompositeMeter

3 participants

@belugabehr @shakuzen @jonatan-ivanov